Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inline poll_states and remove Fuse for vec::merge #79

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

yoshuawuyts
Copy link
Owner

@yoshuawuyts yoshuawuyts commented Nov 15, 2022

Ref #22

Counterpart to #70. Removes the use of Fuse from impl Merge for Vec, and also inlines the poll_states.

Performance

Compared to main
Performance remains roughly comparable.

❯ critcmp main patch  -f vec::merge
group              main                                   patch
-----              ----                                   -----
vec::merge 10      1.04      2.7±0.12µs        ? ?/sec    1.00      2.6±0.05µs        ? ?/sec
vec::merge 100     1.00     43.2±1.93µs        ? ?/sec    1.10     47.5±0.97µs        ? ?/sec
vec::merge 1000    1.02      2.0±0.19ms        ? ?/sec    1.00      2.0±0.15ms        ? ?/sec

Compared to last release

The delta here remains mostly dominated by the move to "perfect" waker semantics:

❯ critcmp patch baseline  -f vec::merge
group              baseline                               patch
-----              --------                               -----
vec::merge 10      1.93      4.9±0.31µs        ? ?/sec    1.00      2.6±0.05µs        ? ?/sec
vec::merge 100     7.60    360.6±3.84µs        ? ?/sec    1.00     47.5±0.97µs        ? ?/sec
vec::merge 1000    18.94    38.0±0.41ms        ? ?/sec    1.00      2.0±0.15ms        ? ?/sec

Copy link
Collaborator

@eholk eholk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I had a couple small suggestions for potential improvements, but I'll leave them up to you if you want to take them. Feel free to merge whenever!

src/stream/merge/vec.rs Show resolved Hide resolved
rng: RandomGenerator,
complete: usize,
wakers: WakerList,
state: PollStates,
done: bool,
len: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is the number of streams we're merging? I might suggest count or num_streams as alternate names, but there's plenty of precedent for using len for this sort of thing, so I'm happy to leave it up to your judgement.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'd suggest adding a len() method that returns self.streams.len() and that way we save 8 bytes off the size of the Merge struct.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I wish we could set methods! - Because we're dealing with a pin projection it generates a struct for us containing the projection. Andy any self methods would need to point to that.

Because we don't control the projection we can't define methods. And so we basically have to define everything in-line. Which is like, another reason why I'd reallllly like us to have safe pin projections, so we can split manually authored futures into separate internal methods etc. Not being able to do this is another reason authoring futures by hand is hard :/

src/stream/merge/vec.rs Show resolved Hide resolved
@yoshuawuyts yoshuawuyts merged commit 2fe5efb into main Nov 16, 2022
@delete-merged-branch delete-merged-branch bot deleted the remove-fuse-vec-merge branch November 16, 2022 14:34
yoshuawuyts added a commit that referenced this pull request Nov 16, 2022
@yoshuawuyts yoshuawuyts mentioned this pull request Nov 16, 2022
yoshuawuyts added a commit that referenced this pull request Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants